Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the blockchain protocol to a headers-only structure with a fixed 112-byte header and a variable-length payload, integrating BFT trust base verification and a new miner reward token system. My review identified a potential issue with the specified git tag for cpp-httplib in CMakeLists.txt and an outdated seed corpus in the fuzzing build script that needs to be updated to match the new header size.
| FetchContent_Declare( | ||
| httplib | ||
| GIT_REPOSITORY https://github.com/yhirose/cpp-httplib.git | ||
| GIT_TAG v0.38.0 |
There was a problem hiding this comment.
The specified git tag v0.38.0 for cpp-httplib does not appear to exist in the official yhirose/cpp-httplib repository. The latest version seems to follow a v0.major.minor scheme (e.g., v0.15.3). This could be a typo and might cause build failures for users who do not have this specific commit cached. Please verify the tag and use a valid, existing tag to ensure build reproducibility.
MastaP
left a comment
MastaP
left a comment
There was a problem hiding this comment.
Code Review: BFT Snapshots
24 inline comments below plus 3 cross-cutting notes. Summary by severity:
- Critical (4): Broken
submitheader/submitblockRPCs, no max payload size,quorum_threshold=0bypass,getrandom()fails on macOS - High (7): Wire format incompatibility, persistence migration,
total_stakeoverflow, HTTP response injection, unbounded CBOR, mining thread crash, unvalidated trust base loading from disk - Medium (6): Hot-path heap allocation, CBlockIndex memory bloat, httplib in public header, secp256k1 context leak, state file permissions, fragile initializer list
- Low (7): Debug string typo, dead typedef, unused include, duplicate code, unnecessary deep copy, redundant member, inconsistent error handling
Cross-cutting issues (not pinned to a specific line):
25. Python functional tests are broken on this branch. consensus_asert_difficulty.py and consensus_timestamp_bounds.py still build 100-byte headers with 20-byte miner field. These will fail.
26. E2E integration tests do not exercise the new payload validation. All tests in test/integration-chain/ use SetBypassPOWValidation(true), which skips CheckBlockHeader entirely. No test verifies the full payload validation pipeline end-to-end.
27. Missing negative tests for payload validation. There is no test that verifies blocks with a mismatched payloadRoot (vs actual payload content) are correctly rejected by CheckBlockHeader.
| if (hex.size() != 200) { | ||
| return util::JsonError("Invalid header length (expect 200 hex chars)"); | ||
| // Expect exactly 224 hex chars (112 bytes) | ||
| if (hex.size() != 224) { |
There was a problem hiding this comment.
Critical: submitheader and submitblock RPCs are broken — external mining is impossible
Both RPCs enforce exactly 224 hex chars (112 bytes = bare header with no payload). However, CheckBlockHeader now requires vPayload.size() >= 32. Any block submitted via these RPCs will have an empty vPayload and fail validation with "bad-payload-size".
Additionally, getblocktemplate does not return payload_root, reward_token_id, or UTB data, so external miners have no way to construct valid blocks even if the size check were fixed.
Suggestion: Accept variable-length hex input (minimum 224 chars for the header + at least 64 chars for the 32-byte token ID hash):
if (hex.size() < 288) { // 112-byte header + 32-byte minimum payload
return util::JsonError("Invalid block data (minimum 288 hex chars: 112-byte header + 32-byte payload)");
}And update getblocktemplate to return the payload fields.
|
|
||
| std::vector<uint8_t> data = nlohmann::json::to_cbor(j); | ||
|
|
||
| // Prepend tag 1013 (d9 03f5) |
There was a problem hiding this comment.
Low: Duplicate CBOR tag prepending in SigBytes() and ToCBOR()
The 0xd9 0x03 0xf5 tag-prepending logic is duplicated between here (lines 139-145) and ToCBOR() (lines 155-161). Extract a helper:
static std::vector<uint8_t> PrependCBORTag1013(std::vector<uint8_t> data) {
data.insert(data.begin(), {0xd9, 0x03, 0xf5});
return data;
}There was a problem hiding this comment.
@ahtotruu has changed the tags, need to check what's the new value is
MastaP
left a comment
MastaP
left a comment
There was a problem hiding this comment.
Re-review after fixes (20 commits since initial review)
Fixed correctly (no further action needed)
| # | Finding | Status |
|---|---|---|
| F2 | MAX_PAYLOAD_SIZE | Fixed — 4096 byte limit with tests |
| F3 | quorum_threshold==0 | Fixed — rejects 0 threshold and empty root_nodes, with overflow checks |
| F4 | getrandom on macOS | Fixed — replaced with cross-platform std::random_device |
| F7 | total_stake overflow | Fixed — overflow check before accumulation |
| F8 | HTTP response body injection | Fixed — truncated to 4096 bytes |
| F9 | Unbounded CBOR | Fixed — MAX_BFT_RESPONSE_SIZE (1 MB) check |
| F12 | Serialize heap alloc | Fixed — new SerializeHeader() returns std::array |
| F14 | httplib in public header | Fixed — pimpl with forward declaration |
| F15 | secp256k1 context leak | Fixed — uses secp256k1_context_static |
| F16 | State file permissions | Fixed — writes with 0600 |
| F17 | Fragile initializer list | Fixed — uses parameter directly |
| F18 | "miner=" in ToString | Fixed |
| F19 | Dead HeaderBytes | Fixed — now used by SerializeHeader() |
| F20 | Unused iostream | Fixed |
| F22 | SigBytes deep copy | Fixed — serializes directly |
| F23 | Redundant latest_trust_base_ | Fixed — uses GetLatestLocked() from map |
| F24 | Inconsistent error handling | Fixed — SyncTrustBases no longer swallows exceptions |
Still open (3 from original + Python tests)
| # | Finding | Severity |
|---|---|---|
| F1 | submitheader still broken (submitblock was fixed) |
Critical |
| F6 | bft_epoch required field breaks backward compat |
High |
| F10 | Mining thread has no top-level exception handler | High |
| F21 | Duplicate CBOR tag code | Low |
| F25 | Python functional tests still broken (submitheader + 100-byte headers) | High |
F11 (trust base disk loading without verification), F13 (vPayload memory bloat) — acknowledged as by-design / future work.
New issues (4 inline comments)
See inline comments below.
|
|
||
| std::vector<uint8_t> data = nlohmann::json::to_cbor(j); | ||
|
|
||
| // Prepend tag 1013 (d9 03f5) |
There was a problem hiding this comment.
Low (from F21, still open): CBOR tag 1013 prepend logic still duplicated
Lines 133-139 (SigBytes) and 149-155 (ToCBOR) contain identical tag-prepending code. Consider extracting a helper:
static std::vector<uint8_t> PrependCBORTag1013(std::vector<uint8_t> data) {
data.insert(data.begin(), {0xd9, 0x03, 0xf5});
return data;
}# Conflicts: # test/integration-network/rpc_integration_tests.cpp
MastaP
left a comment
MastaP
left a comment
There was a problem hiding this comment.
Additional review findings:
-
[P0]
submitheaderstill rejects payload-bearing blocks.HandleSubmitHeaderaccepts inputs longer than 112 bytes, but then rejects any decoded blob whose size is not exactlyCBlockHeader::HEADER_SIZE, so header+payload submissions still fail before validation. -
[P1]
submitheaderis unusable on testnet. The RPC unconditionally callsTestSetSkipPoWChecks(skip_pow), and that helper throws on every non-regtest network even whenskip_powis false. -
[P1] The loader still rejects pre-upgrade chainstate files.
required_fieldsnow requires bothpayload_rootandbft_epoch, so old persisted headers are still rejected asCORRUPTEDinstead of migrating or defaulting. -
[P1] The
HEADERSwire-format change is not version-gated. The protocol version remains unchanged even thoughHEADERSchanged from fixed-size header entries to length-prefixed full block blobs, so incompatible peers can still handshake and only fail later during parsing. -
[P1] The
cpp-httplibpin does not exist upstream.FetchContentstill usesv0.38.0, which is not present on the upstream tags page, so clean builds are not reproducible. -
[P2] The
submitblocknegative tests no longer exercise the parser. Several functional/integration tests now omit the requiredrewardtokenidargument and pass on the usage error path before deserialization or validation runs.
I also verified that several older unresolved threads are fixed in the current branch and will resolve those separately.
3 and 4 are about backwards compatibility, so not relevant. |
|
@lploom CBOR tags needs to be synced with https://github.com/unicitynetwork/unicity-ids/blob/main/cbor-tags.json |
3 participants
Added BFT snapshotting as defined in the Yellowpaper.
New block structure
Removed "minerAddress" (20 bytes) field and added "payloadRoot" field (32 bytes), so the block headers grew from 100 bytes to 112 bytes. In addition, the block now contains variable length payload of hash of miner reward token (32 bytes) and UTB_cbor bytes (variable size depending on number of BFT nodes etc). The payloadRoot is calculated as H(reward_token_hash || UTB_hash).
Reward tokens
Replaced miner addresses with reward tokens. Every time a block is mined, a reward token is generated and stored locally, the hash of the reward token is appended to the block payload. By default the reward tokens are stored to:
$ cat /tmp/node1/reward_tokens.csv
BFT layer integration
If a block is mined then the miner queries BFT layer for new UTBs, if a new UTB is found then it is also appended to block payload. If new UTBs are not found then nothing is appended to the block and zero hash is used to calculate payloadRoot. The BFT node is configurable via --bftaddr flag which defaults to "http://127.0.0.1:25866" in testnet and mainnet. In REGTEST mode the BFT integration is disabled and can be enabled by explicitly setting the --bftaddr flag.